Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit the special case in the [[Set]] algorithm to [OverrideBuiltins] interfaces #715

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Ms2ger
Copy link
Member

@Ms2ger Ms2ger commented Apr 16, 2019

This is the case where it makes most sense, since the prototype chain can't
shadow any properties.

This change does not affect normal usage; it is only detectable by messing with
the prototype of the object.

The majority of implementations (Gecko and Chrome) already follow the proposed
change; WebKit follows the existing spec.

Fixes #630.

… interfaces

This is the case where it makes most sense, since the prototype chain can't
shadow any properties.

This change does not affect normal usage; it is only detectable by messing with
the prototype of the object.

The majority of implementations (Gecko and Chrome) already follow the proposed
change; WebKit follows the existing spec.

Fixes #630.
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there web-platform-tests for this change and do we have a bug against WebKit?

@ExE-Boss
Copy link
Contributor

ExE-Boss commented May 9, 2020

If we do this, then https://github.com/web-platform-tests/wpt/blob/master/webstorage/set.window.js will start failing in the same way as it fails for WebIDL2JS, which currently has a buggy implementation of this (jsdom/webidl2js#218), which causes the special case to always be skipped due to target === receiver always being false in a Proxy trap.

This is because the last test case of https://github.com/web-platform-tests/wpt/blob/master/webstorage/set.window.js currently checks that proto.[[Set]]() isn’t called.

@domenic domenic added the needs tests Moving the issue forward requires someone to write tests label Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Moving the issue forward requires someone to write tests
Development

Successfully merging this pull request may close these issues.

Consider removing call to named property setter in legacy platform object [[Set]]
5 participants